Skip to content

improve _log1pmx_ker internal documentation on the polynomial #97

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

improve _log1pmx_ker internal documentation on the polynomial #97

wants to merge 2 commits into from
+2 −0

Conversation

nsajko
Copy link
Contributor

@nsajko nsajko commented Aug 1, 2025

Note which function the polynomial approximation approximates in a comment in the source code.

Might be useful if someone decided to search for a slightly different polynomial.

@nsajko
improve _log1pmx_ker internal documentation on the polynomial
60a80b5 
Note which function the polynomial approximation approximates in a
comment in the source code.

Might be useful if someone decided to search for a slightly different
polynomial.
@nsajko
Copy link
Contributor Author

nsajko commented Aug 1, 2025

The context here is that I tried to improve upon the existing polynomial by using Sollya, but I could not. Hats off to Oscar (I think the current polynomials were found by Oscar, not sure how he rounded the coefficients to machine precision?!)!

tpapp
tpapp approved these changes Aug 6, 2025
View reviewed changes
Copy link
Collaborator

@tpapp tpapp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@tpapp tpapp requested a review from devmotion August 8, 2025 06:50
devmotion
devmotion reviewed Aug 8, 2025
View reviewed changes
Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand the purpose of the PR - the polynomial and how it was found (with Remez.jl) is already documented in

LogExpFunctions.jl/src/basicfuns.jl

Lines 334 to 339 in 30b15e3

# The kernel of log1pmx
# Accuracy within ~2ulps -0.227 < x < 0.315 for Float64
# Accuracy <2.18ulps -0.425 < x < 0.425 for Float32
# parameters foudn via Remez.jl, specifically:
# g(x) = evalpoly(x, big(2)./ntuple(i->2i+1, 50))
# p = T.(Tuple(ratfn_minimax(g, (1e-3, (.425/(.425+2))^2), 8, 0)[1]))
.

The comment explains that the polynomial is an approximation of $$\sum_{i=0}^{49} 2x^i/(2i + 3)$$ which is a truncation of the power series $$\sum_{i=0}^{\infty} 2x^i/(2i + 3) = 2 (atanh(x) - x) / x^3$$.

@nsajko
Copy link
Contributor Author

nsajko commented Aug 8, 2025

I don't really understand the purpose of the PR - the polynomial and how it was found (with Remez.jl) is already documented

The purpose is to expand the existing documentation with a more clear description. And the closed form is more useful for feeding to a tool like Sollya than the Taylor series is. If you think everything is clear already then I guess there's no improvement from your perspective?

@devmotion
Copy link
Member

I would suggest expanding the existing comment instead of adding a new separate one. I imagine it could be helpful for some readers to know that polynomial g that is approximated here is itself a truncation of the series expansion of 2(atanh(x) - x)/x^3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devmotion devmotion devmotion left review comments

@tpapp tpapp tpapp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nsajko @devmotion @tpapp